(Closes #2202) Update the generation of Fortran declarations to observe dependencies by arporter · Pull Request #3344 · stfc/PSyclone

arporter · 2026-02-23T15:07:30Z

This is a relatively simple PR - it takes the inter-dependence handling out of _gen_param_decls and puts it into gen_decls. This also then means we can delete _gen_param_decls. I've added tests for the various tricky cases described in the original Issue and they all pass now :-)

…ns [skip ci]

codecov · 2026-02-23T15:38:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (7762bd9) to head (8506716).

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3344   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          403       403           
  Lines        57020     57063   +43     
=========================================
+ Hits         57020     57063   +43

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

arporter · 2026-02-23T19:41:20Z

The NEMO ITs failed because of character variables:

Symbol Table of Container 'test_mod':
-------------------------------------
DataSymbol:
  cdwmo: DataSymbol<UnsupportedFortranType('CHARACTER(LEN = ilenwmo), DIMENSION(:), POINTER, PUBLIC :: cdwmo'), Unknown>
  ilenwmo: DataSymbol<Scalar<INTEGER, UNDEFINED>, Static, initial_value=Literal[value:'58', Scalar<INTEGER, UNDEFINED>], constant=True>

(Pdb) cdwmo = table.lookup("cdwmo")
(Pdb) cdwmo.get_all_accessed_symbols()
set()
(Pdb) cdwmo.datatype.declaration
'CHARACTER(LEN = ilenwmo), DIMENSION(:), POINTER, PUBLIC :: cdwmo'

I was proceeding to try and do something with the parse tree in UnsupportedFortranType but actually, it would probably be better to work on populating the partial_datatype in this case.

arporter · 2026-02-25T13:05:38Z

Marking this as blocked for the moment because I want to investigate adding support for a CharacterType with a length property.

arporter · 2026-04-29T08:00:32Z

Although things are now better, complex types are still a problem - I've added a (failing) test which demonstrates this.

arporter · 2026-10-15T09:31:16Z

I had a problem with the UnsupportedTypes created as part of our select-type handling in the frontend. They were missing a partial_datatype so I have added that as part of this PR (to ensure we capture dependencies within their declarations).

arporter · 2026-10-15T11:01:27Z

Unfortunately I had several failures from the ITs, e.g. from the transmutation:

gravity_wave_drag/gw_ussp_mod.f90:28:8:

   28 |     DATA sinphi / 1, 0, -1, 0 /
      |        1
Error: DATA array ‘sinphi’ at (1) must be specified in a previous declaration

and NEMO v5:

  842 |     DATA alpham / 0.095_wp, 0.08_wp, 0.065_wp, 0.065_wp, 0.06_wp, 0.06_wp, 0.06_wp, 0.06_wp, 0.065_wp, 0.075_wp, 0.09_wp, 0.10_wp /
      |        1
Error: DATA array 'alpham' at (1) must be specified in a previous declaration

This might be hard to fix!

We have #2630 but, as Joerg says there, DATA statements with implicit loops will be a fair bit of work. For now, I could add a simple partial datatype that depends on the symbol the data statement is for?

… to capture dependencies

arporter · 2026-10-15T19:46:54Z

A simple partial_datatype wasn't an option as we do type checking etc. when building types. I had to make a StructureType where each component has an initial_value that is a Reference to a dependent Symbol.

arporter · 2026-10-16T08:21:41Z

NEMOv4 IT is failing with signal 11 but I think this is ready for a first look. Functionally it is not a big change but unfortunately it touches lots of tests.
One for anyone :-)

LonelyCat124

@arporter Implementation looks mostly good. I'm slightly concerned that ordering by variable name might make output code harder to read (than e.g. ordering by full declaration string, since that would group variables of the same type together).
Otherwise a few comments on tests/formatting.

LonelyCat124 · 2026-10-16T09:26:16Z

+        # with case-insensitive name comparisons because the dependent symbols
+        # are not always created in the same scope.
+        local_lowered_names = [sym.name.lower() for sym in all_symbols]
+        decln_inputs: dict[str, set[Symbol]] = {}


We don't usually add type hints for variable declarations- this one has made it through. I seem to remember there was some discussion ongoing on this so I'm nmot sure if we made a decision?

There's also some that have appeared in tests (so I don't forget)

LonelyCat124 · 2026-10-16T10:11:58Z

-                if node.field.name == "f3"]) == 1
-    assert len([node for node in first_invoke.schedule.walk(LFRicHaloExchange)
-                if node.field.name == "f4"]) == 1
+    assert (


Can you take a look at these auto-formatted (I assume) asserts? I think they're significantly more difficult to read than the old versions.

LonelyCat124 · 2026-10-16T10:20:18Z

+"""
+    for declaration in expected_declarations.splitlines():
+        if declaration.strip():
+            assert declaration in gen


Does this test the ordering here any more? Or you don't think its relevant for this test as its covered elsewhere?

LonelyCat124 · 2026-10-16T10:23:57Z

            "STENCIL_CROSS, stencil_dofmap_type\n" in result)
    assert ("use flux_direction_mod, only : x_direction, y_direction\n"
            in result)
+    assert "integer(kind=i_def), intent(in) :: f2_extent\n" in result


Why has this been split from output3? I guess its now something like

f2_extent f3_direction f3_extent

? Could still have the old test with the new ordering if so.

LonelyCat124 · 2026-10-16T10:26:21Z

+    # The order of declarations is tested separately. Compare this
+    # transformation's declarations as a set while preserving the exact
+    # order of executable statements.
+#    def sort_declarations(code):


I think these are leftover commented out code?

LonelyCat124 · 2026-10-16T10:39:56Z

        "    type(r2d_field), intent(inout) :: ssh_fld\n"
        "    integer :: j\n"
        "    integer :: i\n"
+        "    type(r2d_field), intent(inout) :: ssh_fld\n"


Has ssh_fld appeared a second timere here?

LonelyCat124 · 2026-10-16T10:42:55Z

        "    integer(kind=i_def) :: cell\n")
    assert expected in code
+    assert "type(field_type), intent(in) :: field1\n" in code
+    assert "type(field_type), intent(in) :: field2\n" in code


Do field1 and field2 no longer appear near each other in the output? If so can we test they appear next to each other as expected. If not, is it worth considering an alternative sorting of non-dependent variables, e.g. sorting by their type as it might keep the Fortran output more readable to have blocks of :

integer :: ... integer : .... ... type(field_type) :: .... type(field_type) :: .... type(field_type) :: ....

? Though I understand that it would be a pain to again change all the tests.

LonelyCat124 · 2026-10-16T10:43:57Z

 from psyclone.tests.lfric_build import LFRicBuild

+
+def assert_lines_present(code, expected):


This file doesn't check ordering of generated code anymore - I'm not necessarily opposed to it just wanted to check you're also ok with that (since most other files still do).

LonelyCat124 · 2026-10-16T10:44:46Z

        "    type(field_type), intent(in) :: m1\n"
        "    type(field_type), intent(in) :: m2\n")
-    assert output in generated_code
+    assert all(line in generated_code for line in output.splitlines())


Also doesn't check ordering now for this test.

LonelyCat124 · 2026-10-16T10:45:16Z

+
+[tool.ruff]
+# Set the maximum line length to 79 characters
+line-length = 79


Is this a local change for your linter?

arporter added 5 commits February 20, 2026 17:05

#2202 use dependence-analysis code from parameter declns for all decl…

c13926c

…ns [skip ci]

#2202 small test fix [skip ci]

9097665

#2202 fix tests

0985ae4

#2202 fix test when dist_mem is False

8f68d2f

#2202 rm _gen_param_decls method

453cd42

#2202 tidy implementation

72f247e

arporter temporarily deployed to integration February 23, 2026 16:08 — with GitHub Actions Inactive

arporter self-assigned this Feb 23, 2026

arporter added bug enhancement in progress labels Feb 23, 2026

#2202 add new (failing) test [skip ci]

06a2702

arporter marked this pull request as draft February 24, 2026 09:05

arporter added the Blocked An issue/PR that is blocked by one or more issues/PRs. label Feb 25, 2026

arporter and others added 6 commits February 25, 2026 13:05

Merge branch 'master' into 2202_decln_ordering

3ad77ac

Merge branch 'master' into 2202_decln_ordering

669c1d4

#2202 fixes post merge

1f815d4

#2202 fix tests after merge

4bca1f3

Merge branch 'master' into 2202_decln_ordering

e1d2f35

Merge branch 'master' into 2202_decln_ordering

1bd9004

arporter removed the Blocked An issue/PR that is blocked by one or more issues/PRs. label Apr 29, 2026

arporter temporarily deployed to integration April 29, 2026 07:19 — with GitHub Actions Inactive

#2202 add explicit test for ordering with declaration of complex type

7c53c0a

LonelyCat124 mentioned this pull request Jun 15, 2026

PSy-layer output not 100% reproducible #3457

Closed

arporter added 2 commits September 2, 2026 11:32

Merge branch 'master' into 2202_decln_ordering

eb3dfdc

#2202 fix tests after merge

075cdbe

#2202 fix doctest failures

5c13124

arporter deployed to integration October 15, 2026 09:45 — with GitHub Actions Active

arporter added 2 commits October 15, 2026 20:29

#2202 give symbols associated with DATA stmts a fake partial_datatype…

29c3405

… to capture dependencies

Merge branch 'master' into 2202_decln_ordering

8506716

arporter deployed to integration October 15, 2026 19:47 — with GitHub Actions Active

arporter requested review from LonelyCat124, hiker and sergisiso October 16, 2026 08:21

arporter added ready for review and removed in progress labels Oct 16, 2026

LonelyCat124 added under review and removed ready for review labels Oct 16, 2026

LonelyCat124 requested changes Oct 16, 2026

View reviewed changes

LonelyCat124 added reviewed with actions and removed under review labels Oct 16, 2026

arporter added 11 commits October 16, 2026 20:31

#2202 WIP re-ordering test output [skip ci]

f0f502f

#2202 tidy change to backend [skip ci]

77dcca8

#2202 fix decln ordering in psyir tests [skip ci]

d75b049

Merge branch 'master' into 2202_decln_ordering

faa9d40

#2202 wip fixing top-level tests [skip ci]

58598ca

#2202 finish fixing top-level tests [skip ci]

7d95084

#2202 fix domain/gocean tests [skip ci]

e2449e5

#2202 WIP fixing domain/lfric tests [skip ci]

b37cb70

#2202 more lfric test fixes [skip ci]

d9eb052

#2202 fix remaining LFRic tests and linting [skip ci]

ca064bb

Merge branch 'master' into 2202_decln_ordering

14e4cfc

		from psyclone.tests.lfric_build import LFRicBuild


		def assert_lines_present(code, expected):

Conversation

arporter commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arporter commented Feb 23, 2026

Uh oh!

arporter commented Feb 25, 2026

Uh oh!

arporter commented Apr 29, 2026

Uh oh!

arporter commented Oct 15, 2026

Uh oh!

arporter commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arporter commented Oct 15, 2026

Uh oh!

arporter commented Oct 16, 2026

Uh oh!

LonelyCat124 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arporter commented Feb 23, 2026 •

edited

Loading

codecov Bot commented Feb 23, 2026 •

edited

Loading

arporter commented Oct 15, 2026 •

edited

Loading